Remove unwraps from metadata decoding: introduce `BlobDecoder` #149455

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Open

jdonszelmann wants to merge 5 commits into rust-lang:main from jdonszelmann:metadata-decoding-s

+662 −547

Contributor

jdonszelmann commented Nov 29, 2025 •

edited

Loading

Collaborator

rustbot commented Nov 29, 2025

These commits modify compiler targets.
(See the Target Tier Policy.)

rustbot added S-waiting-on-review T-compiler labels

jdonszelmann force-pushed the metadata-decoding-s branch from 25b88c6 to cf9344a Compare

November 29, 2025 15:03

Collaborator

rustbot commented Nov 29, 2025

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

Contributor Author

jdonszelmann commented Nov 29, 2025

@bors try @rust-timer queue

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

rust-bors bot added a commit that referenced this pull request


          Auto merge of #149455 - jdonszelmann:metadata-decoding-s, r=<try>

694a8a4

Metadata decoding s

rustbot added the S-waiting-on-perf label

Contributor Author

jdonszelmann commented Nov 29, 2025

cc: @oli-obk this is what I was talking about. not yet clean, but wanted to see what perf looked like :3

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

rust-bors bot commented Nov 29, 2025

💔 Test for 694a8a4 failed: CI. Failed jobs:

try - dist-x86_64-linux (web logs, extended logs)

jdonszelmann force-pushed the metadata-decoding-s branch from cf9344a to bed17cf Compare

November 29, 2025 20:17

Contributor Author

jdonszelmann commented Nov 29, 2025

This comment has been minimized.

Sign in to view

rust-bors bot added a commit that referenced this pull request


          Auto merge of #149455 - jdonszelmann:metadata-decoding-s, r=<try>

7ac6e1b

Metadata decoding s

rust-bors bot commented Nov 29, 2025

☀️ Try build successful (CI)
Build commit: 7ac6e1b (7ac6e1b480efa4bd33f86547b8e73a2122f069d0, parent: 8a3a6bdb68b4d4c9ed922840808b02015741331e)

This comment has been minimized.

Sign in to view

Collaborator

rust-timer commented Nov 29, 2025

Finished benchmarking commit (7ac6e1b): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.2%	[-0.4%, -0.2%]	8
Improvements ✅ (secondary)	-0.4%	[-1.4%, -0.1%]	32
All ❌✅ (primary)	-0.2%	[-0.4%, -0.2%]	8

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 471.041s -> 471.157s (0.02%)
Artifact size: 386.90 MiB -> 386.90 MiB (0.00%)

rustbot removed the S-waiting-on-perf label

Contributor

oli-obk commented Nov 30, 2025

The large workspace benchmark shows improvements in not just instructions, but also time on the write dep info query.

Everything else is a mixed bag on time, unsure why other queries are affected. There are minimal improvements for incremental loading across the board. I'm guessing I'm general the branch predictor just got this all correct.

jdonszelmann added 4 commits

December 1, 2025 12:24


          split out blob decode trait


          make cdata required

b7c5cb0


          remove unnecessary method

72a9d08


          add TyCtxt everywhere replacing nothing or passed sessions

292f156

jdonszelmann force-pushed the metadata-decoding-s branch 2 times, most recently from 524ba79 to 285c9b1 Compare

December 1, 2025 11:26

jdonszelmann commented

View reviewed changes

compiler/rustc_metadata/src/rmeta/decoder.rs

    
                  fn decoder(self, pos: usize) -> Self::Context;

              }

              impl<'a> Metadata<'a> for &'a MetadataBlob {

Contributor Author

jdonszelmann Dec 1, 2025

importantly there are now only two, not 5 implementations of Metadata.

One for MetadataBlob which provides only the BlobDecodeContext which only implements BlobDecoder
One for (tcx, cdata) which provides the MetadataDecodeContext which does implement SpanDecoder and TyDecoder.

This makes it statically known whether we have tcx and cdata available, and whether spans etc can be decoded or not.

jdonszelmann commented

View reviewed changes

compiler/rustc_metadata/src/rmeta/decoder.rs

    
                  fn decode<'a, 'tcx, M: Metadata<'a>>(self, metadata: M) -> T::Value<'tcx>

                  where

                      T::Value<'tcx>: Decodable<DecodeContext<'a, 'tcx>>,

                      T::Value<'tcx>: Decodable<M::Context>,

Contributor Author

jdonszelmann Dec 1, 2025

Decodable specifically with the Context type from whichever kind of M: Metadata is given.

jdonszelmann commented

View reviewed changes

compiler/rustc_metadata/src/rmeta/decoder.rs

    
              impl<'a, 'tcx> MetadataDecodeContext<'a, 'tcx> {

                  #[inline]

                  fn tcx(&self) -> TyCtxt<'tcx> {

                      let Some(tcx) = self.tcx else {

Contributor Author

jdonszelmann Dec 1, 2025

unwrap removed

jdonszelmann commented

View reviewed changes

compiler/rustc_metadata/src/rmeta/decoder.rs

    
                      if let Some(alloc_decoding_session) = self.alloc_decoding_session {

                          alloc_decoding_session.decode_alloc_id(self)

                      } else {

                          bug!("Attempting to decode interpret::AllocId without CrateMetadata")

Contributor Author

jdonszelmann Dec 1, 2025

unwrap removed

jdonszelmann commented

View reviewed changes

compiler/rustc_metadata/src/rmeta/decoder.rs

    
                      let cdata = self.cdata();

                      let Some(sess) = self.sess else {

                          bug!(

Contributor Author

jdonszelmann Dec 1, 2025

unwrap removed

jdonszelmann commented

View reviewed changes

compiler/rustc_metadata/src/rmeta/decoder.rs

    
                      let local_cdata = self.cdata();

                      let Some(sess) = self.sess else {

                          bug!(

Contributor Author

jdonszelmann Dec 1, 2025

unwrap removed

jdonszelmann commented

View reviewed changes

compiler/rustc_metadata/src/rmeta/decoder.rs

    
                      let hi = lo + len;

                      let Some(sess) = decoder.sess else {

                          bug!(

Contributor Author

jdonszelmann Dec 1, 2025

unwrap removed

jdonszelmann commented

View reviewed changes

compiler/rustc_metadata/src/rmeta/decoder.rs

    
                      cfg_version: &'static str,

                  ) -> Result<(), Option<String>> {

                      if !self.blob().starts_with(METADATA_HEADER) {

                          if self.blob().starts_with(b"rust") {

Contributor Author

jdonszelmann Dec 1, 2025 •

edited

Loading

these blob calls were very funny, since it gets an instance of Metadatablob... But these are methods implemented on MetadataBlob.

jdonszelmann commented

View reviewed changes

compiler/rustc_metadata/src/rmeta/decoder.rs

    
                          .decode((self, tcx))

                  }

                  fn load_proc_macro<'tcx>(self, id: DefIndex, tcx: TyCtxt<'tcx>) -> SyntaxExtension {

Contributor Author

jdonszelmann Dec 1, 2025

to make things consistent, I always pass tcx first. There are one or two cases where this meant I moved the existing tcx argument.

Contributor Author

jdonszelmann commented Dec 1, 2025 •

edited

Loading

@oli-obk I think this is ready :) Left you some review notes

This comment has been minimized.

Sign in to view


          document various traits

ca97283

jdonszelmann force-pushed the metadata-decoding-s branch from 285c9b1 to ca97283 Compare

December 1, 2025 12:27

rustbot assigned oli-obk

Collaborator

rustbot commented Dec 1, 2025

oli-obk is not on the review rotation at the moment.
They may take a while to respond.

jdonszelmann changed the title ~~Metadata decoding s~~ Remove unwraps from metadata decoding: introduce BlobDecoder

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review T-compiler